refactor(e2e): setup zrc20 in one transaction #3077
Conversation
gartnera
added
no-changelog
📝 WalkthroughWalkthroughThe pull request introduces modifications primarily to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
gartnera
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
e2e/e2etests/test_whitelist_erc20.go (1)
Line range hint
1-124: Architecture feedback for test structure.While the test is comprehensive, consider these architectural improvements:
- Extract the ERC20 deployment and whitelisting into helper functions to improve reusability across other tests
- Consider using test fixtures for the ERC20/ZRC20 setup and cleanup
Would you like me to provide an example of how to refactor this using test fixtures?
e2e/txserver/zeta_tx_server.go (1)
Line range hint
196-218: Consider using gas estimation instead of scaling gas and fees linearly with message count.Multiplying the gas limit and fees by the number of messages may not accurately reflect the actual gas usage and required fees for the transaction. Consider using gas estimation to calculate the appropriate gas limit and fee amount for the batched messages. This approach can improve efficiency and reduce the likelihood of transaction failures due to insufficient gas.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
cmd/zetae2e/local/local.go(1 hunks)e2e/e2etests/test_whitelist_erc20.go(1 hunks)e2e/txserver/zeta_tx_server.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_whitelist_erc20.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/txserver/zeta_tx_server.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (5)
e2e/e2etests/test_whitelist_erc20.go (1)
53-54: Method signature change looks good.
The update from InitializeLiquidityCap to InitializeLiquidityCaps aligns with the PR's objective of optimizing the setup process by handling multiple transactions efficiently.
Let's verify this API change is consistently applied across the codebase:
✅ Verification successful
Let me gather more information using different search patterns to verify this API change thoroughly.
API change verification complete - Implementation is consistent
The method change from InitializeLiquidityCap to InitializeLiquidityCaps has been properly implemented:
- The implementation exists in
e2e/txserver/zeta_tx_server.gowith correct variadic parameter support - No instances of the old singular method name remain in the codebase
- The method is properly documented and includes error handling
- The test usage in
test_whitelist_erc20.gocorrectly aligns with the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the old method name is not used anywhere else
# and the new method is properly implemented
# Test 1: Check for any remaining usage of old method name
echo "Checking for old method name usage..."
rg "InitializeLiquidityCap[^s]"
# Test 2: Verify the new method implementation
echo "Verifying new method implementation..."
ast-grep --pattern 'func $_($$) InitializeLiquidityCaps($$$)'
Length of output: 269
Script:
#!/bin/bash
# Search for any InitializeLiquidityCap variations more broadly
echo "Searching for InitializeLiquidityCap variations..."
rg -i "initializeliquiditycap" -A 2
# Search for interface or struct definitions containing this method
echo "Searching for interface/struct definitions..."
ast-grep --pattern 'type $_ interface {
$$$
InitializeLiquidityCaps($$$)
$$$
}'
# Search for the actual implementation
echo "Searching for method implementations..."
ast-grep --pattern 'func ($_ $_) InitializeLiquidityCaps($$$) {
$$$
}'
Length of output: 1275
cmd/zetae2e/local/local.go (1)
Line range hint 192-196: Validate keygen status even when setup is skipped.
The current implementation assumes keygen is completed when setup is skipped. This assumption could lead to issues if incorrect. Consider adding a validation check.
-if !skipSetup {
+// Always verify keygen status, but only wait for completion during setup
+resp, err := deployerRunner.ObserverClient.Keygen(ctx, &observertypes.QueryGetKeygenRequest{})
+noError(err)
+if resp.Keygen == nil || resp.Keygen.Status == observertypes.KeygenStatus_PendingKeygen {
+ if !skipSetup {
waitKeygenHeight(ctx, deployerRunner.CctxClient, deployerRunner.ObserverClient, logger, 10)
+ } else {
+ logger.Print("⚠️ Warning: Keygen is pending but setup is skipped")
+ }
}e2e/txserver/zeta_tx_server.go (3)
244-246: Properly handling broadcast failures when the transaction code is non-zero.
Good addition to catch and handle transaction failures based on the response code. This ensures that errors are reported immediately, enhancing the robustness of the transaction broadcasting process.
271-273: Consistent error handling for transaction failures based on result code.
Appropriate inclusion of error checking for resTx.TxResult.Code. This consistency in error handling improves the reliability of the transaction process.
598-611: Efficiently initializing liquidity caps for multiple ZRC20 tokens in a single transaction.
This refactoring improves performance by batching the initialization of liquidity caps, reducing transaction overhead and network load. Well-implemented and aligns with best practices for efficient batch processing.
Description
Deploy zrc20 contracts in one transaction to increase e2e setup speed. This reduces setup time by ~20 seconds. Also:
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor